-
-
Notifications
You must be signed in to change notification settings - Fork 124
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Use the same options object in handle-response as in send-request #148
Conversation
Instead of cloning this.options again in handle-response.js, pass the options object from send-request.js. This way, pjax.state.options will also have the request options.
lib/proto/attach-form.js
Outdated
@@ -41,7 +41,7 @@ var formAction = function(el, event) { | |||
// jscs:disable disallowImplicitTypeConversion | |||
if (!!element.name && element.attributes !== undefined && tagName !== "button") { | |||
var type = element.attributes.type | |||
// jscs:enable disallowImplicitTypeConversion |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you meant to remove this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. The tests run fine without it, so it's not necessary.
I should have added it to #147, but I forgot to do it before I merged it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you're explicitly disabling a test for a given code block with an inline comment, I think you should always re-enable it after to ensure that you don't accidentally lose coverage... or, alternatively, just disable the rule globally if you don't find it to be of any use.
In this case, // jscs:disable disallowImplicitTypeConversion
should be paired with a // jscs:enable disallowImplicitTypeConversion
somewhere or we should disable the rule altogether, IMO. Personally, I'd be quite happy to disable it, as I find !!element.name
to be quite a convenient shorthand for casting to a Boolean.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My bad. I just glanced quickly at it and I thought they were both disable
rules, applicable to only the next line.
I'll revert that. Thanks for catching that.
Additionally, once this is merged, how do you feel about pushing out another release? I think there's been some significant improvements and fixes since v0.2.5 that would be great to get out there. |
I agree. I think we should wait a few days to see if any other issues come up, and then we can push v0.2.6 and start working on v0.3.0/v1.0 |
@robinnorth Is there anything else that needs to be done here before merging? |
lib/proto/handle-response.js
Outdated
tempOptions.request = request | ||
module.exports = function(responseText, request, href, options) { | ||
options = options || clone(this.options); | ||
options.request = request |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree it's useful to have the request available in this.state.options
and in the options passed to the various Pjax events that are triggered, but perhaps the options object that is passed around should still be a clone to guard against accidental mutation of this.options
by end users consuming the options object.
i.e.
options = options || clone(this.options)
options.request = request
var tempOptions = clone(this.options)
//...
this.state.options = tempOptions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure which scenario you're trying to preempt.
Do you mean:
options = options ? clone(options) : clone(this.options)
options.request = request
//...
this.state.options = options
In case handleResponse()
is called with this.options
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What I'm trying to avoid is having this.options
passed directly when events are triggered, as this PR changes trigger(document, "pjax:error", tempOptions)
to trigger(document, "pjax:error", options)
, for example. Passing the options object directly could allow this.options
to be mutated accidentally. The snippet you posted would prevent that from happening.
lib/proto/handle-response.js
Outdated
var tempOptions = clone(this.options); | ||
tempOptions.request = request | ||
module.exports = function(responseText, request, href, options) { | ||
options = options || clone(this.options); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like an errant semicolon slipped through the net in a previous commit 😉
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I much prefer semicolons. I think it makes the code easier to read, and prevents any ASI errors. Unless there is objection, I would like to add them all back in at some point, so I'll just leave this here for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also much prefer them too, so that's fine by me.
lib/proto/attach-form.js
Outdated
@@ -41,7 +41,7 @@ var formAction = function(el, event) { | |||
// jscs:disable disallowImplicitTypeConversion | |||
if (!!element.name && element.attributes !== undefined && tagName !== "button") { | |||
var type = element.attributes.type | |||
// jscs:enable disallowImplicitTypeConversion |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you're explicitly disabling a test for a given code block with an inline comment, I think you should always re-enable it after to ensure that you don't accidentally lose coverage... or, alternatively, just disable the rule globally if you don't find it to be of any use.
In this case, // jscs:disable disallowImplicitTypeConversion
should be paired with a // jscs:enable disallowImplicitTypeConversion
somewhere or we should disable the rule altogether, IMO. Personally, I'd be quite happy to disable it, as I find !!element.name
to be quite a convenient shorthand for casting to a Boolean.
Sorry, I just realised that my code review was still 'pending'. Have published now so that you can see my comments. |
Instead of cloning
this.options
again inhandle-response.js
, pass the options object fromsend-request.js
. This way,pjax.state.options
will also have the request options.